I believe that it's injected so that it's easier to test, because you can write a very simple mock function rather than a full blown implementation of TransitService.

Constructor injection is the recommended way to inject services - no need to comment, if not it should be commented.

It looks like it's only used in one place, and I didn't spot any uses in testing. But maybe I missed something, or it's done this way to facilitate potential future testing. There may also have been a judgement that TransitService was just too broad as the SiriTripPatternCache only needed access to this one function. In which case this is a sort of anonymous sub-interface to better encapsulate the workings of TransitService.

I think this is about decoupling, a low level thing should not point to a high level thing because that would create cyclic dependencies. It make it difficult test, but also to reason about. In general I am fan of creating a Functonal interface in these situation witch can be used to document the feature. There is no need for the TransitService to implement this interface, the injector can do the mapping.

I prefer the linked version as this changes the docs here, when the class is renamed.

IntelliJ has an option to extend rename operations to strings, comments etc. Where class names are clearly distinct from any plain text (via use of CamelCase) it may be sufficient to use the names.

This may be a question of style and uniformity more than functionality, since an IDE can readily identify all instances of the class name. There are about 19 references in comments without the link notation. If we change some of them we should probably change all of them. Extending that to every class name in every comment might arguably lead to a decrease in readability or maintainability.

Of course if it is already a policy to progressively introduce link in any new comments then we should update these.

This is discussed in the dev-meeting. There are pros/cons for both approches. Using the linking is good because it provide hover JavaDoc, but doing it the first time a class is mentioned is enough. Also, in this case the coupling is so strong that it it probably just noice. When refactoring we should always check stings as well. Note! Avoid using link if it introduce an illegal dependency (e.g. linking to the OTP internal transit model from Raptor).

Tip! I use LiveTemplates to speed up typing: co-> {@code } and li->{@link }. (Example template: {@link $SELECTION$$END$})

Java doc should not be added to private fields, but the get (or set) accessor.

In this case, the private field can only be set via the constructor. The comment can be moved to the constructor but then for a maintainer working on the internals of this class to understand what the earlyStart field does, they'd need to find that it's set in the constructor, read the constructor javadoc, and confirm that the documented constructor parameter sets this field.

I get the idea of wanting the documentation concentrated on public methods, but I think it depends whether the documentation is intended for developers working on this code, or for developers making use of the code. In a case like this I'd be inclined to duplicate the documentation - maintenance can cross a line to being very tedious when the reader has to go searching for the meaning of every field.

Some DRY could be achieved by linking the private field Javadoc to the only place it's set, but this requires a verbose approach that is just as long as the comment explaining the purpose of the field, and much more fragile/full of irrelevant detail:
/** @see #SiriAlertsUpdateHandler(String, TransitModel, TransitAlertService, SiriFuzzyTripMatcher, Duration) */

I also find there's something non-generalizable about a convention of documenting on an accessor method because the relationship from fields to methods that mutate them is one-to-many. There's not one unambiguous place to put the documentation (on a setter, a factory method, one of several constructors?) but there is a single field they're all referencing.

I moved it to an @see annotation on the constructor javadoc.

The feed scoped ID of this TransitAlert will be the single feed ID
associated with this update handler, plus the situation number provided in the SIRI feed.

The strategy for handling IDs should be documented in a more visible place than on a private method. I also think we probably need to allow more than one strategy - so it should be extracted so it is pluggable. This is probably one of the main things we need to fix to more this from the Sandbox to the main code.

From a maintenance perspective, this may be a good place to document the structure of the IDs because they are created during this mapping operation. Although I value DRY, the same concern exists here as other places above: increasing visibility of docs for people reusing/configuring the encapsulated code might make it hard for the maintainer to find those descriptions and use them to quickly understand the actions of this method.

I have also put the information in the class-level Javadoc, and rephrased the information for the two contexts. Also included a note that FeedScopedId creation strategy should be pluggable or configurable as a condition for moving out of sandbox.

In a large production system, ideally all the scheduled data would be integrated into a single data source with a unified ID namespace, and all the realtime data would also be integrated into a single data source with an exactly matching namespace.

I disagree, it does not scale - e.g. putting all transit data for the world into the same source is not going to work. We have done it in Norway, and there are many advatiges - but there are also many problems(scaling, performance, resource allocation, cost). But I do agree that IDs should be unique - this requirment can be met by convention. Currently OTP add a feed prefix to ids to make them unique inside OTP, but this is not optimal because IDs should be unique and the same across whole system and in TIME. OTP only have limited support for entity versioning of NeTEx quays and StopPlaces.

I wasn't considering a whole-world case, where many other things about OTP also wouldn't scale. I was considering current typical OTP instances, where the largest ones are usually for single countries like Norway, Germany, or France and many deployments are much smaller.

Here I was trying to express something that I heard in past discussion: Certain responsibilities are outside the scope of OTP, and I was under the impression most of us considered a data integration pipeline to be an external service upstream of OTP. Similar to how realtime prediction is also seen as ideally outside and upstream of OTP.

If this is not the case, and integration of multiple data sources within OTP itself is considered advantageous (rather than just a convenience feature) I would like to document why and how. Is the key fact that some data sources update more frequently than others, and they update at different times? So rather than producing and ingesting the whole integrated data feed each time one of the constituent feeds changed, are you thinking of live OTP instances live-patching large chunks of their internal data? Basically, entire feeds would be replaced in the same way as realtime messages are currently applied? If this is the case we should begin documenting these ideas in the realtime working group doc.

On ID namespaces: If all the IDs across many feeds are guaranteed to be unique, in some sense they are already using a unified namespace. The "upstream integration system" I mentioned doesn't have to be merging the feeds - it could also just be data consistency and referential integrity checks for example.

I do not think we disagree here, I am not talking about "live-patching large chunks of their internal data". If you do a single netex data source or multiple files it both comes with a set of challenges you need to deal with - one way is not better than the other - in my eyes. I also think that adding a feedScope to the ID should be an opt in feature - the id is changed. It divide the ecosystem in "downstream" and "upstream" of OTP.

I think this is about decoupling, a low level thing should not point to a high level thing because that would create cyclic dependencies. It make it difficult test, but also to reason about. In general I am fan of creating a Functonal interface in these situation witch can be used to document the feature. There is no need for the TransitService to implement this interface, the injector can do the mapping.

copyOf() Naming convention

It is good to know where the "Layer" comes from, after working with OTP for many years I did not know. I think we should stick to industry/de facto standards and not invent over own when it comes to architecture naming(not transit domain). I can recommend Domain Driven Design (E.Evens) and Applying UML and Patterns: An Introduction to Object-Oriented Analysis and Design and the Unified Process (C.Larman). These books are a bit old, so not sure what they say on n-tier architecture (layered architecture) - In DDD inversion of control i used to avoid the classic n-tier - to protect the domain logic/main asset. This is useful(or necessary) in complex components like OTP.

Model responsibilites:

• manage and encapsulate aggreagates, entities and domain data (p.s. data not logic)
• Models store data somewhere (usually via a persistance-layer(DB), but in OTP in we store it in memory andby serializing it to disk).
• In OTP the model serve to groups of clients: Those who read data (API and route engine) and those who read/write data(graphBuiler/updaters). These two type og clients should get access to ONLY the features they need - without access to e.g. writing. Since OTP is all about routing, the model should be tailored for that purpose. If something is getting in the way, it should be extracted and stored somewhere else - maybe in its own model.
• data integrity - enforcing business rules related to data.
• support for use-case specific functionality, but not responsible for it.
• support for none-functional requirements.

The model is not responsible for:

• Use-case specific rules and data structures. If use-case specific features become "noice" or introduce complexity, then they are best kept outside the model.

I do not think we disagree here, I am not talking about "live-patching large chunks of their internal data". If you do a single netex data source or multiple files it both comes with a set of challenges you need to deal with - one way is not better than the other - in my eyes. I also think that adding a feedScope to the ID should be an opt in feature - the id is changed. It divide the ecosystem in "downstream" and "upstream" of OTP.